Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Mar 26, 2025

What do these changes do?

ReDoc

The api-server previously used REST to communicate with the catalog, but those connections are now deprecated. A new CatalogService has been added to the api-server, built on the catalog_rpc interface.

Currently, CatalogService is only used in tests. It will be adopted in the main code in a future PR, at which point the web clients will be removed.

Details of the changes are as follows:

  • api-server micro-service
    • new CatalogService to represent the "service layer" that interacts with the catalog micro-service
      • implemented using an RPC client servicelib.rabbitmq.rpc_interfaces.catalog.services.py for comms
      • adapts catalog's rpc schema models to api-server's domain models.
        • These can later be translated into rest schema models Solver, Program ?
    • NOTE that this PR still does NOT use the CatalogService in the rest-handlers
  • catalog micro-service
    • extends rpc api with get_service_release_history(key) -> list[ServiceRelease]
    • deprecates some rest api for services

Related issue/s

How to test

cd services/api-server
make install-dev
pytest -vv tests/unit/test_services_catalog.py 
cd services/catalog
make install-dev
pytest -vv tests/unit/with_dbs/test_api_rpc.py

Dev-ops

None

@pcrespov pcrespov self-assigned this Mar 26, 2025
@pcrespov pcrespov added a:catalog catalog service a:apiserver api-server service labels Mar 26, 2025
@codecov
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 79.01235% with 17 lines in your changes missing coverage. Please review.

Project coverage is 87.44%. Comparing base (0d58ad8) to head (28039ae).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7439      +/-   ##
==========================================
- Coverage   87.45%   87.44%   -0.01%     
==========================================
  Files        1717     1711       -6     
  Lines       66614    66493     -121     
  Branches     1132     1132              
==========================================
- Hits        58259    58147     -112     
+ Misses       8034     8025       -9     
  Partials      321      321              
Flag Coverage Δ
integrationtests 65.11% <ø> (-0.06%) ⬇️
unittests 86.62% <79.01%> (-0.01%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 93.90% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 92.05% <100.00%> (+<0.01%) ⬆️
pkg_notifications_library 84.57% <ø> (ø)
pkg_postgres_database 88.11% <ø> (ø)
pkg_service_integration 70.03% <ø> (ø)
pkg_service_library 72.86% <0.00%> (-0.11%) ⬇️
pkg_settings_library 90.78% <ø> (ø)
pkg_simcore_sdk 85.46% <ø> (ø)
agent 96.46% <ø> (ø)
api_server 90.76% <100.00%> (+0.07%) ⬆️
autoscaling 96.08% <ø> (ø)
catalog 91.82% <80.00%> (-0.32%) ⬇️
clusters_keeper 99.24% <ø> (ø)
dask_sidecar 91.25% <ø> (ø)
datcore_adapter 98.11% <ø> (ø)
director 76.61% <ø> (-0.10%) ⬇️
director_v2 91.27% <ø> (-0.03%) ⬇️
dynamic_scheduler 97.33% <ø> (ø)
dynamic_sidecar 90.11% <ø> (ø)
efs_guardian 89.79% <ø> (ø)
invitations 93.28% <ø> (ø)
payments 92.66% <ø> (ø)
resource_usage_tracker 89.12% <ø> (ø)
storage 86.04% <ø> (-0.35%) ⬇️
webclient ∅ <ø> (∅)
webserver 85.87% <ø> (-0.04%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d58ad8...28039ae. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pcrespov pcrespov force-pushed the is7421/upgrade-catalog-service-listing branch from bbfe7e6 to 6df7f75 Compare March 26, 2025 19:55
@pcrespov pcrespov changed the title WIP: ✨ Is7421/upgrade catalog service listing ✨ New CatalogService in api-server that connects via rpc to the catalog micro-service Mar 27, 2025
@pcrespov pcrespov marked this pull request as ready for review March 27, 2025 00:51
@pcrespov pcrespov requested a review from bisgaard-itis March 27, 2025 00:51
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool. but please consider my questions.

**RESPONSE_MODEL_POLICY,
deprecated=True,
description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q1: how are we going to handle "cancel_on_disconnect" with RPC calls?
Q2: I really want to move to faststream, for documentation purposes, and it seems they also have RPC (over Redis). but still I am wondering about Q1. cause we currently have the issue that we do not check connection status.

Currently the server side of the RPC will not cancel a call where the client timed-out or closed the connection for whatever reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually faststream also allows RPC over rabbitmq

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RPC in fast stream is actually new. It was not there when I last checked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanderegg

From what I've gathered on the rabbitmq documentation page there is no guarantee of cancellation on remote (server side) by design. https://www.rabbitmq.com/docs/direct-reply-to#limitations

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'd have to check if faststream implemented any support for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually faststream also allows RPC over rabbitmq

I tried this out at some point. It seemed quite easy to use. But (at least at the time I tried it) it didn't support (de)serialization via type-hints fastapi-style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q1: how are we going to handle "cancel_on_disconnect" with RPC calls?

Good point.

If this is not implemented in the rpc-client by default, I am indeed not taking care of that.

Nonehtless, all the rpc operations in this PR are reading operations. If client disconnects, it is indeed not optimal but it is not dramatic. It might be a bigger problem if they are write operations. Moreover, in the case of cancel_on_disconnect is enabled, write operations also must guarantee some "atomicity" as well e.g. using units-of-work, to avoid inconsistent states.

Q2: I really want to move to faststream, for documentation purposes, and it seems they also have RPC (over Redis). but still I am wondering about Q1. cause we currently have the issue that we do not check connection status.

that would be great! I am all ears ... happy to hear your thoughts about it offline

Currently the server side of the RPC will not cancel a call where the client timed-out or closed the connection for whatever reason.

**RESPONSE_MODEL_POLICY,
deprecated=True,
description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RPC in fast stream is actually new. It was not there when I last checked

**RESPONSE_MODEL_POLICY,
deprecated=True,
description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanderegg

From what I've gathered on the rabbitmq documentation page there is no guarantee of cancellation on remote (server side) by design. https://www.rabbitmq.com/docs/direct-reply-to#limitations

**RESPONSE_MODEL_POLICY,
deprecated=True,
description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you'd have to check if faststream implemented any support for it.

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks a lot. Thanks also for not using the new catalog client yet in the api-server. That would have caused a lot of conflicts with what I am doing for the studies with associated data.



@pytest.fixture
def mocked_rpc_catalog_service_api(mocker: MockerFixture) -> dict[str, MockType]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add this in pytest-simcore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well not really because it mocks an object in the simcore_service_api_server

image

That said, that object is the catalog_rpc client that is in servicelib. Nonetheless, when it comes to mocks, the "path of import" is very important, even if you are mocking the same object. That is, i am not 100% sure that it would work if I patch the object importing instead from servicelib

We can check at some other point.

**RESPONSE_MODEL_POLICY,
deprecated=True,
description="Use instead rpc._service.list_services_paginated -> PageRpcServicesGetV2",
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually faststream also allows RPC over rabbitmq

I tried this out at some point. It seemed quite easy to use. But (at least at the time I tried it) it didn't support (de)serialization via type-hints fastapi-style.

@pcrespov pcrespov added this to the The Awakening milestone Mar 27, 2025
@pcrespov pcrespov force-pushed the is7421/upgrade-catalog-service-listing branch from 3d32b27 to 1b60257 Compare March 27, 2025 13:32
@pcrespov pcrespov merged commit c535784 into ITISFoundation:master Mar 27, 2025
25 checks passed
@sonarqubecloud
Copy link

@pcrespov pcrespov deleted the is7421/upgrade-catalog-service-listing branch March 27, 2025 13:47
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Apr 15, 2025
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:apiserver api-server service a:catalog catalog service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants